Check dict item accesses where it isn't already checked #2863
Conversation
| #endif | ||
| } | ||
|
|
||
| inline PyObject * dict_getitem(PyObject *v, PyObject *key) |
There was a problem hiding this comment.
Why isn't this a member of py::dict?
There was a problem hiding this comment.
Because the code that is intended to call this (function dispatcher) uses the raw CPython API, presumably for performance reasons?
|
Also, if someone could retrigger the checks for this, it's likely that the tests will pass. |
| #endif | ||
| } | ||
|
|
||
| inline PyObject * dict_getitem(PyObject *v, PyObject *key) |
| throw error_already_set(); | ||
| } | ||
|
|
||
| rv = PyDict_GetItemWithError(v, kv); |
There was a problem hiding this comment.
At this point, can't you just call dict_getitemstring?
There was a problem hiding this comment.
Nope, still has potential to raise an exception. See https://bugs.python.org/issue35459
As copied from cpython @ https://github.com/python/cpython/blob/d5fc99873769f0d0d5c5d5d99059177a75a4e46e/Objects/dictobject.c#L1545
There was a problem hiding this comment.
My bad, I was thinking about calling your dict_getitem. From this line on, dict_getitemstring and dict_getitem are the same.
There was a problem hiding this comment.
Not quite the same, Py_DECREF needs to be called on the key before the error is thrown.
virtuald
force-pushed
the
error-checks
branch
from
eb98f16 to
a14ee3c
Compare
EricCousineau-TRI
left a comment
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Looks great from my side! Just a minor nit on rationale comment. (Yeah, discoverable via git blame + clicks, but mayhaps a terse statement could help!)
|
Hi Dustin, sorry for the long delay! This PR escaped me before. It looks very important. |
|
Nice, thanks! Good to see that the new clang-tidy is happy. |
|
I forgot to mention, I already integrated this PR for Google-internal global testing. I'll hold off merging until I get the results, later tonight if things go well. |
|
The Google-internal testing was successful. Merging! |
5 participants
Description
Suggested changelog entry: